Add option to generate test operations in diff
#154
Conversation
This reverts commit 13e9ab0.
|
Hi, any news on this? |
There was a problem hiding this comment.
Pull Request Overview
This PR adds a new generate_test_ops option to the JSON patch generation that creates test operations before every remove and replace operation. This enhances data integrity by capturing the previous values before modifications are applied.
- Adds
generate_test_opsparameter tomake_patch()andJsonPatch.from_diff()methods with default valueFalse - Modifies
DiffBuilderto generate test operations when the flag is enabled - Updates test suite to run all existing tests with both
generate_test_ops=Trueandgenerate_test_ops=False
Reviewed Changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| jsonpatch.py | Adds generate_test_ops parameter support and logic to generate test operations in the diff builder |
| tests.py | Updates all test cases to use the new parameter and adds new test classes to verify behavior with test operations enabled |
| @@ -475,6 +481,11 @@ def apply(self, obj): | |||
|
|
|||
| return obj | |||
|
|
|||
| def _on_undo_add(self, path, key): | |||
There was a problem hiding this comment.
These empty methods _on_undo_add and _on_undo_remove in the base PatchOperation class appear to be default implementations that don't perform any logic. Consider adding docstrings to explain their purpose or making them abstract methods if they're meant to be overridden by subclasses.
Copilot uses AI. Check for mistakes.
| :param generate_test_ops: While :const:`True` generate test operations | ||
| capturing previous values of `replace`/`remove` | ||
| operations. By default do not generate these |
There was a problem hiding this comment.
The docstring uses 'While :const:True' which is grammatically incorrect. It should be 'When :const:True' or 'If :const:True'.
| :param generate_test_ops: While :const:`True` generate test operations | |
| capturing previous values of `replace`/`remove` | |
| operations. By default do not generate these | |
| :param generate_test_ops: If :const:`True`, generate test operations | |
| capturing previous values of `replace`/`remove` | |
| operations. By default, do not generate these |
Copilot uses AI. Check for mistakes.
| @@ -641,6 +652,12 @@ def from_diff( | |||
| :param dst: Data source document object. | |||
| :type dst: dict | |||
|
|
|||
| :param generate_test_ops: While :const:`True` generate test operations | |||
There was a problem hiding this comment.
Same grammatical error as in the make_patch function. The docstring uses 'While :const:True' which should be 'When :const:True' or 'If :const:True'.
| :param generate_test_ops: While :const:`True` generate test operations | |
| :param generate_test_ops: If :const:`True`, generate test operations |
Copilot uses AI. Check for mistakes.
| @@ -801,6 +819,11 @@ def _item_added(self, path, key, item): | |||
| for v in self.iter_from(index): | |||
| op.key = v._on_undo_remove(op.path, op.key) | |||
|
|
|||
| if self.generate_test_ops: | |||
| prev_op_index = index[0] | |||
| if isinstance(prev_op_index[2], TestOperation): | |||
There was a problem hiding this comment.
[nitpick] The code accesses prev_op_index[2] directly using a magic index. Consider using a named constant or a more descriptive way to access the operation object to improve code readability.
Copilot uses AI. Check for mistakes.
| exp = [{'op': 'remove', 'path': '/foo/0/baz'}] | ||
|
|
||
| if self.generate_test_ops: | ||
| exp.insert(0, {'path': '/foo/0/baz', 'value': 2, 'op': 'test'}) |
There was a problem hiding this comment.
[nitpick] The expected result is defined before the conditional logic that modifies it. Consider moving the base expectation definition closer to where it's used, or make the conditional logic more explicit about what's being added.
| exp = [{'op': 'remove', 'path': '/foo/0/baz'}] | |
| if self.generate_test_ops: | |
| exp.insert(0, {'path': '/foo/0/baz', 'value': 2, 'op': 'test'}) | |
| if self.generate_test_ops: | |
| exp = [ | |
| {'path': '/foo/0/baz', 'value': 2, 'op': 'test'}, | |
| {'op': 'remove', 'path': '/foo/0/baz'} | |
| ] | |
| else: | |
| exp = [{'op': 'remove', 'path': '/foo/0/baz'}] |
Copilot uses AI. Check for mistakes.
Hi,
This PR adds a new option for generating patches -
generate_test_ops- that adds atestoperation before everyremoveandreplaceoperation. This allows for generating patches that have stronger data integrity characteristics. It is also a useful option if you care about knowing what the data was pre-patch (this is important for our use-case). The flag defaults toFalseto keep the default behavior the same.I have modified the test suite to run the
MakePatchTestCasetwice - once withgenerate_test_ops=Trueand once withgenerate_test_ops=False. Patches are still applying properly in both cases so I am confident that all is well.Please let me know if there's anything else that needs to be done as part of this PR.
Thanks!